New Attribute Syntax (#2569) #12538

thehydroimpulse · 2014-02-25T08:51:53Z

This implements the currently leading (inner) attribute syntax (#2569) while keeping backwards compatibility. Everything compiles and passes (including two new tests).

New Syntax:

inner: #![foo(bar)] (without a semicolon)
outer: stays the same.

The new syntax also supports semicolons for backward compability.

thehydroimpulse · 2014-02-25T08:56:44Z

Oops, forgot to remove debug print statements.

brson · 2014-02-26T01:19:16Z

I feel good about this solution, and commend you @thehydroimpulse for taking the initiative to push it through.

@pnkfelix Can you deal without the semis?

brson · 2014-02-26T01:20:55Z

@pcwalton You may have an opinion too.

thehydroimpulse · 2014-02-26T02:20:25Z

@brson Added two warning statements when using the old syntax. (One for the missing ! and the other for a useless ;)

huonw · 2014-02-26T02:25:48Z

I think the warnings should be commented out until a snapshot and the main codebase has been migrated to the new syntax, or every build will be really noisy.

thehydroimpulse · 2014-02-26T06:24:40Z

@huonw The builds were definitely way too noisy, so I commented the warnings.

pnkfelix · 2014-02-26T09:18:40Z

@brson if the brackets stay, i can live without the semis. (it's not worth further belaboring at that point.)

If there is any chance of the square brackets going away, then I will want the semis back. But right now we continue to have a clear end-delimiter, So it's okay.

brson · 2014-02-26T22:10:35Z

This needs a test that a #![foo] on the first line of a file is interpreted as an attribute and not a shebang. That test will need an // ignore-license.

thehydroimpulse · 2014-02-26T22:52:59Z

@brson Added the shebang test. Not sure if it's correct or not, but everything passes.

alexcrichton · 2014-02-27T00:52:18Z

src/libsyntax/parse/attr.rs

+                    inner_attr_bang = true;
+                    if !permit_inner {
+                        self.fatal("An inner attribute was not permitted in this context.");
+                    }


There should be a test for this case (inner attribute when you're not expecting it)

Ahh, yes.

Is there a prime case for when it's not expecting one? For example, not at the top of some file/module.

fn main() {} #![lang(foo)] fn foo() {}

Something like that?

Looks like a good test to me.

BTW, error messages should start with lower case (i.e. "an inner ...")

@huonw Good to know, thanks.

alexcrichton · 2014-02-27T00:59:05Z

I think @brson was thinking of a test that looks like:

#![some_outer_attribute]
// ignore-license

fn main() {}

Also, I don't think this should change the pretty printer right now (because we're just accepting the #![ syntax, not changing to it), but it would be nice for this to identify the relevant location in the pretty printer to get modified when this merges and gets a snapshot. Not required, but would be nice to have!

thehydroimpulse · 2014-02-27T01:00:54Z

@alexcrichton Ahh, ok. I'll fix that.

brson · 2014-02-27T01:54:49Z

I think this is the ideal test case:

#![main]
// ignore-license
fn foo() {}

Makeing the attribute something real will prevent a false-negative that just parses the attribute as a shebang and throws it away.

huonw · 2014-02-27T01:57:54Z

Preferably the other tests would do the same:

mod foo {}
mod foo {
     #![cfg(this_will_never_occur)]
}
fn main() {}

then if the cfg isn't parsed correctly there will be two mod foo's and the compiler will throw an error.

thehydroimpulse · 2014-02-27T03:45:54Z

@brson @alexcrichton Added a new commit addressing all of the comments. I'll squash my commits once everything is good.

bstrie · 2014-02-27T14:49:14Z

If we're going to make further changes to the attribute syntax by removing brackets, I think it would make more sense to do it all at once.

I also disagree with pnkfelix, in that I don't think that bracket-less attributes need semicolons to delimit them. They're perfectly readable without the trailing delimeter, and using semicolons there just leads people into thinking that they're statements.

liigo · 2014-02-27T15:44:42Z

2014年2月27日下午10:49于 "Ben Striegel" notifications@github.com写道：

If we're going to make further changes to the attribute syntax by
removing brackets, I think it would make more sense to do it all at once.

I also disagree with pnkfelix, in that I don't think that bracket-less
attributes need semicolons to delimit them. They're perfectly readable
without the trailing delimeter, and using semicolons there just leads
people into thinking that they're statements.

+1
—
Reply to this email directly or view it on GitHub.

pnkfelix · 2014-02-27T15:44:42Z

@bstrie I guess we will just have to agree to disagree on this point. I've already posted my reasoning behind why using semicolons as a terminator is not inconsistent (in a comment on #2569), and you and I have already posted our disagreement on this point there. At this point I'm sorry I even brought it up again.

liigo · 2014-02-27T16:07:49Z

Even without the brackets and semicolon, we can still parse attributes
syntax properly. I don't know why need them. Please remove them all, which
are useless at all.
2014年2月27日下午11:44于 "Felix S Klock II" notifications@github.com写道：

@bstrie https://github.com/bstrie I guess we will just have to agree to
disagree on this point. I've already posted my reasoning behind why using
semicolons as a terminator is not inconsistent (in a comment on #2569http://../2569#issuecomment-21098112).
At this point I'm sorry I even brought it up again.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12538#issuecomment-36255215
.

pnkfelix · 2014-02-27T16:28:01Z

@liigo Until #11886 lands, one needs a terminator to tell whether an attribute is #!foo = bar or if it is just #!foo. Now, that terminator could be the end-of-line, which may be what you were thinking. But as noted in previous comments we do not currently treat end-of-line as significant except for // comments.

(The above note is just about parser-ambiguities; a separate topic from the question of user-experience, which I am also concerned about, but is more subjective and/or requires user testing for which we do not have resources currently.)

(Also, for outer attributes, the item itself acts as a the terminator, which I believe is why the previous bracket-less suggestion terminated by semi-colons would have "worked." But at this point I have said I am fine with the approach outlined on this ticket.)

brson · 2014-02-28T22:11:39Z

@thehydroimpulse I've told you wrong on the attr-shebang test. What we're trying to test for is that rust doesn't interpret an inner attribute on the first line as a shebang (Rust ignores shebangs), but the test case I gave you uses an outer attr instead of inner.

Something like this might be more appropriate:

#![allow(unknown_features)]
#![feature(bogus)]
fn main() { }

The code for parsing shebangs is here.

From reading it, it will interpret the the first two bytes of a file being '#!' as a shebang. I think the rule should be more like: if the first two characters are '#!' and the first three characters not counting whitespace are not '#![', then discard the first line as a shebang.

Note that unfortunately there's a second shebang parser here that is just used for pretty printing. That also needs to be changed to follow the same rule.

flaper87 · 2014-03-04T09:56:42Z

src/libsyntax/parse/attr.rs

+                        self.fatal("an inner attribute was not permitted in this context.");
+                    }
+                } else {
+                    warned = true;


I don't think warned is necessary. The message here is different from the warn below. Although the refer to the same new syntax, they are warning about different things.

@flaper87 There was two warning calls for using the old syntax. The second warn is just about the extra semicolon though, so I see your point. But, any old syntax will now print one warning instead of two.

alexcrichton · 2014-03-10T20:13:30Z

@thehydroimpulse, it looks like @brson's comment about shebangs still needs to get addressed. If you're running low on time, though, I can take over if you want!

thehydroimpulse · 2014-03-10T20:18:10Z

Yup. I just need to push my changes which I'll do that today.
On Mar 10, 2014 2:13 PM, "Alex Crichton" notifications@github.com wrote:

@thehydroimpulse https://github.com/TheHydroImpulse, it looks like
@brson https://github.com/brson's comment about shebangs still needs to
get addressed. If you're running low on time, though, I can take over if
you want!

Reply to this email directly or view it on GitHubhttps://github.com//pull/12538#issuecomment-37228418
.

Signed-off-by: Daniel Fagnan <dnfagnan@gmail.com>

…ail test.

thehydroimpulse · 2014-03-14T00:33:32Z

@alexcrichton I've been getting the same error from Travis locally, too. Not sure what's going on. Rebase, re-configuring, and cleaning doesn't fix it.

/home/travis/build/mozilla/rust/src/libstd/lib.rs:72:14: 72:34 error: can't find crate for `rustuv`
/home/travis/build/mozilla/rust/src/libstd/lib.rs:72 #[cfg(test)] extern crate rustuv;

alexcrichton · 2014-03-14T07:31:45Z

src/libsyntax/parse/lexer.rs

+
+// EFFECT: Peek 'n' characters ahead.
+pub fn peek(rdr: &StringReader, n: uint) -> Option<char> {
+    let offset = byte_offset(rdr, rdr.pos.get()).to_uint() + (n - 1);


This seems to be a dubious method, because this isn't looking n characters ahead, but rather n bytes ahead.

Ahh, makes sense. I basically followed what nextch had, but also having the ability to specify more than one to be peeked. This was the simpler option to forget/rollback.

alexcrichton · 2014-03-14T07:33:48Z

That test failure looks like the attributes aren't getting parsed correctly. Looking at the code, I'm not entirely sure why, but I would recommend dumping the AST of some small programs to take a look at what's happening with the attributes.

alexcrichton · 2014-03-14T07:34:55Z

src/libsyntax/parse/attr.rs

+
+        let style = if permit_inner {
+
+            if self.eat(&token::SEMI) {


Judging by the previous if statement, it looks like this should be structured as if permit_inner && self.eat(&token::SEMI) rather than permit_inner always parsing an inner attribute.

This logic should also change, the semicolon no longer dictates whether it's an inner attribute, something will need to get done when the ! is parsed.

alexcrichton · 2014-03-20T17:40:21Z

@thehydroimpulse it'd be awesome to get this in for the 0.10 release (coming up soon-ish), would you like me to rebase and take over?

thehydroimpulse · 2014-03-20T17:57:20Z

Yeah, I botched my setup on this branch and can't seem to rebuild it. So
go ahead and take it over if you wish.
On Mar 20, 2014 11:40 AM, "Alex Crichton" notifications@github.com wrote:

@thehydroimpulse https://github.com/TheHydroImpulse it'd be awesome to
get this in for the 0.10 release (coming up soon-ish), would you like me to
rebase and take over?

Reply to this email directly or view it on GitHubhttps://github.com//pull/12538#issuecomment-38197949
.

alexcrichton · 2014-03-20T18:01:50Z

Ok, thanks for your work!

brson mentioned this pull request Feb 26, 2014

Accept strings as values of attributes #12467

Closed

alexcrichton reviewed Feb 27, 2014
View reviewed changes

flaper87 reviewed Mar 4, 2014
View reviewed changes

alexcrichton mentioned this pull request Mar 10, 2014

Change #[lang="foo"]-style items to #[lang("foo")] #11886

Closed

thehydroimpulse added 8 commits March 13, 2014 16:23

Added new attribute syntax with backward compatibility.

22f3838

Signed-off-by: Daniel Fagnan <dnfagnan@gmail.com>

Switched to the new leading candidate. Keeping backward compability.

6abffcd

Removed print calls

e6b4258

Added warning calls for use of old syntax

340f729

Signed-off-by: Daniel Fagnan <dnfagnan@gmail.com>

Fixed formatting error. Commented out the warnings for the time being.

e21c2b1

added shebang test

bbf3ddb

Signed-off-by: Daniel Fagnan <dnfagnan@gmail.com>

Removed redundant token checking. Added shebang test. Added compile-f…

5f13fe2

…ail test.

Fixed tests. Added .peek() method.

736cdec

alexcrichton reviewed Mar 14, 2014
View reviewed changes

alexcrichton closed this Mar 20, 2014

New Attribute Syntax (#2569) #12538

New Attribute Syntax (#2569) #12538

Uh oh!

Conversation

thehydroimpulse commented Feb 25, 2014

Uh oh!

thehydroimpulse commented Feb 25, 2014

Uh oh!

brson commented Feb 26, 2014

Uh oh!

brson commented Feb 26, 2014

Uh oh!

thehydroimpulse commented Feb 26, 2014

Uh oh!

huonw commented Feb 26, 2014

Uh oh!

thehydroimpulse commented Feb 26, 2014

Uh oh!

pnkfelix commented Feb 26, 2014

Uh oh!

brson commented Feb 26, 2014

Uh oh!

thehydroimpulse commented Feb 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Feb 27, 2014

Uh oh!

thehydroimpulse commented Feb 27, 2014

Uh oh!

brson commented Feb 27, 2014

Uh oh!

huonw commented Feb 27, 2014

Uh oh!

thehydroimpulse commented Feb 27, 2014

Uh oh!

bstrie commented Feb 27, 2014

Uh oh!

liigo commented Feb 27, 2014

Uh oh!

pnkfelix commented Feb 27, 2014

Uh oh!

liigo commented Feb 27, 2014

Uh oh!

pnkfelix commented Feb 27, 2014

Uh oh!

brson commented Feb 28, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 10, 2014

Uh oh!

thehydroimpulse commented Mar 10, 2014

Uh oh!

thehydroimpulse commented Mar 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 20, 2014

Uh oh!

thehydroimpulse commented Mar 20, 2014

Uh oh!